Skip to content

Allow multiple LogStorage with primary and secondaries #417

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
jgreffe wants to merge 36 commits into from
Closed

Allow multiple LogStorage with primary and secondaries #417

jgreffe wants to merge 36 commits into from

Conversation

@jgreffe
Copy link

@jgreffe jgreffe commented Jul 15, 2025 •
edited
Loading

This PR is a follow-up to #406 (comment).

Impacted downstream changes are:

This PR adjusts the LogStorageFactory API for two main purposes:

  • To allow users to explicitly select a particular LogStorageFactory in case they have multiple plugins installed that implement LogStorageFactory
  • To allow the implementation of a new TeeLogStorageFactory, which allows users to configure a "primary" LogStorageFactory that handles reads and writes, and a "secondary" factory that only handle copies of all writes
    • This prevents plugins from having to reimplement their own Tee logic internally (as done by the existing opentelemetry plugin)
    • It also allows the implementation of write-only LogStorage implementations that simply want to copy log content elsewhere without reading it back into Jenkins, as considered in Security-2401 Fix credentials leakage to Splunk splunk-devops-plugin#17

Specifically, this PR converts LogStorageFactory from an ExtensionPoint to a Describable, and introduces LogStorageFactoryDescriptor. It also introduces a GlobalConfiguration for Pipeline logging and a new TeeLogStorageFactory logger implementation.

Monosnap.screencast.2025-07-18.15-01-23.mp4

This change is not backwards compatible. All plugins that implement LogStorageFactory will need to be updated and released shortly after this PR is released to avoid compatibility issues.

Testing done

See new and existing automated tests here and in downstream PRs. This PR has also been tested manually.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

dwnusbaum
dwnusbaum reviewed Jul 16, 2025
View reviewed changes
@jgreffe jgreffe force-pushed the jgreffe/multiple-log-storage branch from 269b2ee to bd88176 Compare July 18, 2025 14:57
dwnusbaum
dwnusbaum reviewed Jul 18, 2025
View reviewed changes
src/main/java/org/jenkinsci/plugins/workflow/log/tee/TeeBuildListener.java
}

@Override
public void close() throws Exception {
Copy link
Member

@dwnusbaum dwnusbaum Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to note: While I was looking at the code yesterday I realized the logic here is a bit confusing. We close the delegate TaskListeners, but we don't actually close the TeeOutputStream or the PrintStream returned by getLogger(). At first I thought maybe we could simplify by just calling getLogger().close(), which would eventually close the TeeOutputStream and we could get rid of the rest of the method, but at least BufferedBuildListener and CloudWatchSender have special close methods that do more than just closing the internal streams, so we really do need to try to close the TaskListeners themselves.

This means that after a call to close(), getLogger() and outputStream will still exist as unclosed streams even after their delegates have been been closed, which is a bit weird. We might be able to call getLogger().close() and then close the listeners, but I can't remember if any of the relevant streams have problems with being closed twice. I would maybe look into this a bit just to make sure whether the TeeOutputStream not being closed is ok.

Copy link
Author

@jgreffe jgreffe Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I update with something like:

        Exception exception = null;
        if (outputStream != null) {
            try {
                outputStream.close();
            } catch (IOException e) {
                exception = e;
            }
        }

then TeeLogStorageTest#smokes fails with:

org.opentest4j.MultipleFailuresError: Multiple Failures (2 failures)
	org.junit.ComparisonFailure: expected:<[<a href='http://nowhere.net/'>nikde</a>
]> but was:<[]>
	java.lang.AssertionError: 
Expected: is "starting\none #1\ntwo #1\ntwo #2\ninterrupting\none #2\none #3\npausing\nresuming\none #4\nthree #1\nending\n"
     but: was "starting\none #1\ntwo #1\ntwo #2\ninterrupting\none #2\none #3\npausing\nresuming\none #4\nthree #1\nending\nha:////4AM+Xbq6l0DXt+Aa5ectJ4m2Ny0f1G1cNAXESjnHxoh7AAAAlB+LCAAAAAAAAP9b85aBtbiIQSajNKU4P08vOT+vOD8nVc+jsiC1KCczL9svvyTVzHb1RttJBUeZGJg8GdhyUvPSSzJ8GJhLi3JKGIR8shLLEvVzEvPS9YNLijLz0q0rihik0IxzhtAgwxgggJGJgaGiAMhgLWEQzigpKbDS18/LL89ILUrVy0st0QcAFd2f8JgAAAA=nikde\n"
	...
	Suppressed: org.junit.ComparisonFailure: expected:<[<a href='http://nowhere.net/'>nikde</a>
]> but was:<[]>
		at org.jenkinsci.plugins.workflow.log.LogStorageTestBase.assertLog(LogStorageTestBase.java:324)
		at org.jenkinsci.plugins.workflow.log.LogStorageTestBase.assertStepLog(LogStorageTestBase.java:305)
		at org.jenkinsci.plugins.workflow.log.LogStorageTestBase.smokes(LogStorageTestBase.java:155)

Copy link
Member

@dwnusbaum dwnusbaum Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you specifically need to use getLogger().close(), not outputStream.close, maybe so that the PrintStream flushes its buffer and the stream, but I am not exactly sure why the former works but the latter does not without investigating more deeply. If things are ok without closing the PrintStream or TeeOutputStream itself that might be fine too, I would just check to make sure everything is getting GC'd as expected. Maybe there could be a difference in behavior if you tested writing lines without newlines or something like that, but I do not know.

Copy link
Author

@jgreffe jgreffe Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see da22a49#diff-7edd00ed1b12a0d1e07adc240c962db00534506ab3043d8636f193ba84d7436cR51

Copy link
Author

@jgreffe jgreffe Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But now with the additional getLogger().close(), the logger is flushed and closed, making TeeOutputStreamTest#primary_fails_close fail, and the close() method is called twice:

Breakpoint reached
	at org.jenkinsci.plugins.workflow.log.tee.TeeOutputStreamTest$4.close(TeeOutputStreamTest.java:118)
	at org.jenkinsci.plugins.workflow.log.tee.RemoteCustomFileLogStorage$Writer.close(RemoteCustomFileLogStorage.java:138)
	at org.jenkinsci.plugins.workflow.log.tee.TeeOutputStream$$Lambda/0x000000d80168a838.apply(Unknown Source:-1)
	at org.jenkinsci.plugins.workflow.log.tee.TeeOutputStream.handleAction(TeeOutputStream.java:45)
	at org.jenkinsci.plugins.workflow.log.tee.TeeOutputStream.close(TeeOutputStream.java:34)
	at java.io.PrintStream.implClose(PrintStream.java:500)
	at java.io.PrintStream.close(PrintStream.java:484)
	at org.jenkinsci.plugins.workflow.log.tee.TeeBuildListener.close(TeeBuildListener.java:51)
...
Breakpoint reached
	at org.jenkinsci.plugins.workflow.log.tee.TeeOutputStreamTest$4.close(TeeOutputStreamTest.java:118)
	at org.jenkinsci.plugins.workflow.log.tee.RemoteCustomFileLogStorage$Writer.close(RemoteCustomFileLogStorage.java:138)
	at org.jenkinsci.plugins.workflow.log.tee.RemoteCustomFileLogStorage$MyListener.close(RemoteCustomFileLogStorage.java:99)
	at org.jenkinsci.plugins.workflow.log.tee.TeeBuildListener.close(TeeBuildListener.java:55)
	at org.jenkinsci.plugins.workflow.log.tee.TeeOutputStreamTest.primary_fails_close(TeeOutputStreamTest.java:123)
	at java.lang.invoke.LambdaForm$DMH/0x000000d801218c00.invokeVirtual(LambdaForm$DMH:-1)
	at java.lang.invoke.LambdaForm$MH/0x000000d801324000.invoke(LambdaForm$MH:-1)
	at java.lang.invoke.Invokers$Holder.invokeExact_MT(Invokers$Holder:-1)

Copy link
Member

@dwnusbaum dwnusbaum Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry, I was not saying that we definitely need it, only that if we do need it, we need to close the full PrintStream, not just the OutputStream, and yes it means the stream will be closed twice as I mentioned in #417 (comment). It's just not clear to me whether it is preferable to leave the outer streams open and just let them be cleaned up by GC after the delegate TaskListeners are closed, or to close the PrintStream, causing the TeeOutputStreamTest stream to be closed twice. I will try to investigate in more detail later today.

Copy link
Member

@dwnusbaum dwnusbaum Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so now we close the stream twice, but everything seems to be ok IIUC. @jgreffe did you investigate to see if you prefer the behavior with the current code or how you had things prior to adding line 51?

dwnusbaum
dwnusbaum reviewed Jul 21, 2025
View reviewed changes
@dwnusbaum dwnusbaum mentioned this pull request Jul 23, 2025
Closed
@jgreffe jgreffe force-pushed the jgreffe/multiple-log-storage branch from 2b775e6 to da22a49 Compare July 24, 2025 06:52
@jgreffe jgreffe mentioned this pull request Jul 29, 2025
Closed
6 tasks
kellie-freeman
kellie-freeman reviewed Jul 31, 2025
View reviewed changes
Copy link

@kellie-freeman kellie-freeman left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some suggestions for the strings, but probably didn't catch all instances. Overall, I think the use of "Tee" and "factories" is a bit confusing, so I added some possible alternatives. I'll be OOTO until August 11th, so I added several options in most cases, depending on the route we may take.

dwnusbaum
dwnusbaum reviewed Aug 1, 2025
View reviewed changes
src/main/java/org/jenkinsci/plugins/workflow/log/tee/TeeBuildListener.java
}

@Override
public void close() throws Exception {
Copy link
Member

@dwnusbaum dwnusbaum Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so now we close the stream twice, but everything seems to be ok IIUC. @jgreffe did you investigate to see if you prefer the behavior with the current code or how you had things prior to adding line 51?

@jgreffe jgreffe force-pushed the jgreffe/multiple-log-storage branch from 3a985fc to b707a36 Compare August 5, 2025 07:54
This was referenced Aug 5, 2025
Closed
Draft
dwnusbaum
dwnusbaum reviewed Aug 5, 2025
View reviewed changes
dwnusbaum
dwnusbaum reviewed Aug 6, 2025
View reviewed changes
@jgreffe jgreffe mentioned this pull request Aug 8, 2025
Closed
6 tasks
kellie-freeman
kellie-freeman reviewed Aug 22, 2025
View reviewed changes
Copy link

@kellie-freeman kellie-freeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a few more minor string suggestions. If there are any that I missed, please let me know.

@jgreffe jgreffe requested review from dwnusbaum and jglick September 4, 2025 06:40
@jgreffe jgreffe closed this Oct 1, 2025
@jgreffe
Copy link
Author

jgreffe commented Oct 1, 2025

Closing as blocked by downstream PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jglick jglick Awaiting requested review from jglick

@dwnusbaum dwnusbaum Awaiting requested review from dwnusbaum

2 more reviewers

@Vlatombe Vlatombe Vlatombe left review comments

@kellie-freeman kellie-freeman kellie-freeman left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

breaking

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jgreffe @jglick @Vlatombe @dwnusbaum @kellie-freeman